-
Notifications
You must be signed in to change notification settings - Fork 1
Add 3D Windfield #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 3D Windfield #13
Conversation
|
You can see the docu, based on this PR here: https://opensourceawe.github.io/AtmosphericModels.jl/previews/PR13/ |
baptistelabat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First part of review
| /Manifest.toml | ||
| .vscode/settings.json | ||
| data/*.npz | ||
| examples/Manifest.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear to me how the Manifest.toml are used. In python project, I am used to commit the .lock files so that it is possible to define a default configuration for the tests or to ensure to get reproducible results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A Manifest.toml file is created when you first install the project, for example by doing:
julia --project
using Pkg
Pkg.instantiate()
If a manifest file exists, Pkg.instantiate() installs exactly all dependencies listed in the manifest.
In contrast, Pkg.update() looks at the Project.toml file and updates all dependencies to the newest versions allowed by the Project.toml file.
In the last commit I added two default manifests that document the last tested set of packages used, Manifest-v1.10.toml.default and Manifest-v1.11.toml.default for Julia 1.10 and Julia 1.11.
Normally these are not needed, Project.toml should be sufficient, but if something brakes (because a minor package update was buggy) you can always do
cp Manifest-v1.11.toml.default Manifest-v1.11.toml
julia --project
using Pkg; Pkg.instantiate()
And you have the last know good set of packages installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it clear to you now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes clear, but I feel like only one version could be supported as default. The julia version number is integrated in the Manifest.toml, so this should be sufficient, avoiding to have to rename the file ? After, you may save other manifest in subfolder for example in you have published a study (but I think this should not be in a package, but wise if the publication topic is the package).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, renaming of the file is needed. If you start Julia, it looks first for a Manifest file that has its own version number in the file name. If that is found, it uses it. If there is no such file, it uses Manifest.toml (without version number). And if there is no such file it creates it.
We always want to support two versions, the latest Julia version with long term support (which is 1.10), at the latest stable version (after we found it mature enough). Before making a release I always run both the unit tests and the examples on both Julia version. Switching the Julia version is easy, for example by executing:
juliaup default +1.10
Ok, once you need to do juliaup add 1.10.
If you only have one Manifest.toml file without the version number in the file name and switch the Julia version you are using, then things get messed up.
1-Bart-1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 🥇
When creating a windfield the following message is shown
[ Info: Creating wind field for 3.483 m/s. This might take 30s or more...
I would expect to get a message with the amount of time used after creating the wind field as well.
I left some comments.
The interactive plot is very nice!
baptistelabat-syroco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slowly sharing my progress on the review.
| /Manifest.toml | ||
| .vscode/settings.json | ||
| data/*.npz | ||
| examples/Manifest.toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes clear, but I feel like only one version could be supported as default. The julia version number is integrated in the Manifest.toml, so this should be sufficient, avoiding to have to rename the file ? After, you may save other manifest in subfolder for example in you have published a study (but I think this should not be in a package, but wise if the publication topic is the package).
Uh oh!
There was an error while loading. Please reload this page.